Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix pkg_resources, scipy.ndimage, and pytest deprecation warnings #829

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

moustakas
Copy link
Member

This PR cleans up a number of deprecation warnings seen in the unit tests due to:

The pkg_resources warnings aren't seen in the desitarget tests because tests are only done against python/3.9, but they are seen in other packages like fastspecfit, which are testing against python/3.10 and 3.11.

@moustakas moustakas requested a review from geordie666 August 22, 2024 14:09
@coveralls
Copy link

coveralls commented Aug 22, 2024

Coverage Status

coverage: 53.047% (-0.003%) from 53.05%
when pulling 47d5429 on fix-deprecation-warnings
into 971d9d5 on main.

@moustakas
Copy link
Member Author

moustakas commented Aug 22, 2024

@geordie666 I accidentally updated doc/changes.rst in main (while belatedly checking to make sure I had the most recent commits), but then made the same change here. Let me know if you want me to fix main (sorry!).

Also, coverage tests are failing (not just here, but for some time), but I'm not sure why.

@geordie666
Copy link
Contributor

geordie666 commented Aug 22, 2024

Excellent, thanks for these updates. Reviewing now. I don't think it's an issue that you updated changes.rst in main. That doc file is always easy to merge/revert.

@geordie666
Copy link
Contributor

I think everything looks good @moustakas, and only have one suggestion/request:

Could you change constructions like this (there are quite a few so I didn't want to flag them all):

cmd = resources.files('desitarget').joinpath('urat/fortran/v1dump')

to use os.path.join() or something similar? I know that we mostly only support nix-like operating systems in desitarget but it's conceivable that some functions are useful on other OS's that don't use / as the path separator.

I'm not sure why the coverage tests are failing. It hasn't been a high priority to dig into, but we should open an issue for completeness.

@moustakas
Copy link
Member Author

Could you change constructions like this (there are quite a few so I didn't want to flag them all):
cmd = resources.files('desitarget').joinpath('urat/fortran/v1dump')
to use os.path.join() or something similar? I know that we mostly only support nix-like operating systems in desitarget but it's conceivable that some functions are useful on other OS's that don't use / as the path separator.

@geordie666 I'm sorry to push back, but I think this is unnecessary busy work for me and I don't think I'd prioritize getting to these changes (whereas the deprecation warnings are making it annoying for me to identify genuine errors and other relevant warnings in fastspecfit).

In the spirit of making the minimal set of changes in this PR, I simply inherited the use of / as the path separator currently in desitarget/main, so if we want to make this change, I recommend we open a new ticket and try to make these changes in a different PR.

Hope that's alright.

@geordie666
Copy link
Contributor

That's fine. Go ahead and merge. I'll do the busywork in my next branch.

@moustakas moustakas merged commit 6edde80 into main Aug 22, 2024
1 check passed
@moustakas moustakas deleted the fix-deprecation-warnings branch August 22, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants